Style package with new {mlr.styler} package #626

pat-s · 2021-04-12T19:13:41Z

This is a first diff of the new {mlr.styler} package (still private) by @lorenzwalthert.
Lorenz put a lot of time into our mlr-style, not only by creating this new package but also in the past.

It should make updating + maintenance more easy and should also serve as an exemplary package for additional styles next to the tidyverse_style.
Beforehand we used a fork of mine.

@mllg I am not sure if you use {styler} in practice for {mlr3} but your input on specific re-occurring patterns which are changed in the diff is appreciated.
Remember that we can create the style guide to be passive/non-invasive so that in some cases multiple solutions are allowed / not force styled.

Some years have passed since we evaluated the initial guide. If some of your coding patterns have changed since there, it would now be a good time to discuss these here.

Also cc @berndbischl @mb706 @be-marc @pfistfl @jakob-r @RaphaelS1 (your input is appreciated/desired as well of course as the style applies to all mlr-org repos. {mlr3} is just the demo-repo for now.).

@lorenzwalthert
If there are any new transformers that you know of which would be possibly helpful here, just suggest them :)

pat-s · 2021-04-12T19:15:58Z

R/BenchmarkResult.R

-    }
-  ),
-
+    }),


@mllg

IIRC we discussed about this pattern/change the last time already and agreed to allow both options?

Tested this on another repo and in our old mlr-style this was not changed by {styler} -> @lorenzwalthert can we remove this styling? :)

Indeed, there was a transformer for that,but for some reason, it was commented out in the revision of your fork I used when c/p the transformers. Adressed in mlr-org/styler.mlr#7.

Is this a possible misunderstanding?

I meant to say that we want to allow the

} )

pattern. And avoid a styling into }).
I think this is why I commented out this transformer at some point.

I see that in the pull you explicitly commented it in again.
Assuming that it was commented out in this current draft - why did this styling happen then? 🤔
Or am I on the wrong path?

Yup, there was a misunderstanding (and also merge conflicts). We want to activate the rule that adds a line break if there is none and remove the rule that removes the line break. Force pushed to mlr-org/styler.mlr#7 after rebasing on main.

R/DataBackend.R

pat-s · 2021-04-12T19:19:02Z

R/LearnerClassifDebug.R

-          error_predict        = p_dbl(0, 1, default = 0, tags = "predict"),
-          error_train          = p_dbl(0, 1, default = 0, tags = "train"),
-          message_predict      = p_dbl(0, 1, default = 0, tags = "predict"),
-          message_train        = p_dbl(0, 1, default = 0, tags = "train"),
-          predict_missing      = p_dbl(0, 1, default = 0, tags = "predict"),
+          error_predict = p_dbl(0, 1, default = 0, tags = "predict"),
+          error_train = p_dbl(0, 1, default = 0, tags = "train"),
+          message_predict = p_dbl(0, 1, default = 0, tags = "predict"),
+          message_train = p_dbl(0, 1, default = 0, tags = "train"),
+          predict_missing = p_dbl(0, 1, default = 0, tags = "predict"),


@lorenzwalthert

IIRC we had support for this in the old fork - was there a specific transformer or did we do a hacky island solution here?

alignment is supported in general with {styler}, but only with right-alignment:

styler.mlr::style_text( 'call( x = 2, uz = 333 ) ' ) #> call( #> x = 2, #> uz = 333 #> )

^{Created on 2021-04-13 by the reprex package (v1.0.0)}

I suggest to manually align them to one of the supported alignments in https://styler.r-lib.org/articles/detect-alignment.html to make sure styler does not destroy the formatting of deliberately aligned code. Of course, we could also work on supporting the above alignment style. PRs to styler welcome.

Left alignment support on the way: r-lib/styler#774

I force pushed a full pkg styling with the most recent status of {mlr.styler}.

I assume we need to import the recent changes from upstream styler to get the left alignment working?

Other than that, it looks pretty good!

You need to have the devel version of styler with 9f9bdff at least yes. I am adding a minimal version requirement to {styler.mlr} aud bump the devel version in {styler}.

Done. I suggest to ~~re-style~~ git reset and re-style and force push (re-style won't restore previous alignment).

👍

resetted and restyled - some more alignment takes it seems :)

lorenzwalthert · 2021-04-12T20:29:11Z

@pat-s thanks, will look into it tomorrow. Alternatively, you could make one PR per scope, starting with spaces, instead of all of it together. Maybe it’s easier to review. I did that in my fork, you can cherry pick one after the other.

R/ResamplingBootstrap.R

mllg · 2021-04-13T09:35:23Z

R/BenchmarkResult.R

@@ -215,6 +218,7 @@ BenchmarkResult = R6Class("BenchmarkResult",
    #'
    #' @return [data.table::data.table()].
    aggregate = function(measures = NULL, ids = TRUE, uhashes = FALSE, params = FALSE, conditions = FALSE) {
+


I'm not a big fan of enforcing a newline here. Any reason to keep this?

I can't find a reference anymore in the style guide but at some point the idea was to leave a blank line when the function declaration is long. Assuming you have a function declaration somewhere that has a blank line at the top, would you want to remove it or simply not touch it? If we don't touch it, we also can't touch

{ # code }

easily (because of the inner workings of styler). A compromise would be to allow up either one or zero blank lines in any of these two curly brace expressions, but not more.

I can't find a reference anymore in the style guide

The style guide might also not be 100% up-to-date 😅

I think being deliberate here is probably best - i.e. allowing both.

A compromise would be to allow up either one or zero blank lines in any of these two curly brace expressions, but not more.

Sounds like a good compromise to me (if that is easier for {styler} also). @mllg?

I think we never want to have two blank lines anywhere and any of zero or one is fine?

RaphaelS1 · 2021-04-13T17:26:06Z

Just a short comment for now. Points 11 and 17 in the mlr style guide seem to have evolved to the opposite so return statements are rare and we decided to explicitly use :: for most packages except mlr3 and paradox (e.g. we tend to explicitly write mlr3misc::)

pat-s · 2021-04-14T08:42:34Z

Just a short comment for now. Points 11 and 17 in the mlr style guide seem to have evolved to the opposite so return statements are rare and we decided to explicitly use :: for most packages except mlr3 and paradox (e.g. we tend to explicitly write mlr3misc::)

Yeah, the guide is probably not up-to-date or followed fully in practice.
I think {styler} does not touch semantic things like adding/removing return() statements IIRC.

The same goes for explicit namespacing I think - this is probably also not trivial and would require quite some magic to infer this from the function name.

But yeah, we should probably adjust this in the guide.

pat-s · 2021-04-26T12:45:27Z

R/TaskClassif_penguins.R

-    new = c("bill_length",    "bill_depth",    "flipper_length",    "body_mass")
+    new = c("bill_length", "bill_depth", "flipper_length", "body_mass")


@lorenzwalthert DO you have an idea why this alignment is not honoured?

Because spacing is only honoured one level deep. This is two level deep (function call in function call). Sorry, styler can't do this. Use strict = FALSE if you want to preserve one more more space here (but it comes with other implications as shown in the dedicated vignette).

R/bibentries.R

pat-s · 2021-04-26T12:46:03Z

R/mlr_reflections.R

-    ~type,     ~package, ~task,         ~learner,         ~prediction,         ~measure,
-    "regr",    "mlr3",   "TaskRegr",    "LearnerRegr",    "PredictionRegr",    "MeasureRegr",
-    "classif", "mlr3",   "TaskClassif", "LearnerClassif", "PredictionClassif", "MeasureClassif"
+    ~type, ~package, ~task, ~learner, ~prediction, ~measure,
+    "regr", "mlr3", "TaskRegr", "LearnerRegr", "PredictionRegr", "MeasureRegr",
+    "classif", "mlr3", "TaskClassif", "LearnerClassif", "PredictionClassif", "MeasureClassif"


@lorenzwalthert DO you have an idea why this alignment is not honoured?

Because left alignment only works with =, not with ~. You must right align here. See lorenzwalthert#2 (comment).

Edit: this should be resolved with latest styler.

Would adding support for ~ be lots of work? I don't do these alignments personally but I think it would be import for @mllg

I tried for a few hours and I did not get to work it easily... Maybe I can look into it again since we are on it.

This is really niche so really think about if you want to do this :)

Would be great and appreciated of course though 👍

Ok, I found a way to do it and merged it into master: r-lib/styler#785. After the other alignment things were implemented properly, it seemed much easier than I thought initially. Are you happy now? 😀 Please try it out in your example and let me know, I think it should work. I also bumped min version req for {styler} in {styler.mlr} to avoid future destruction.

Thanks! Just restyled but it seems it not yet working 🙈

Because you have an extra wurst with .key = '...' -.-. Fix on the way...

Please try again xD

This seems resolved with latest styling, right?

R/bibentries.R

lorenzwalthert · 2021-04-26T13:55:55Z

R/mlr_reflections.R

-    ~type,     ~package, ~task,         ~learner,         ~prediction,         ~measure,
-    "regr",    "mlr3",   "TaskRegr",    "LearnerRegr",    "PredictionRegr",    "MeasureRegr",
-    "classif", "mlr3",   "TaskClassif", "LearnerClassif", "PredictionClassif", "MeasureClassif"
+    ~type, ~package, ~task, ~learner, ~prediction, ~measure,
+    "regr", "mlr3", "TaskRegr", "LearnerRegr", "PredictionRegr", "MeasureRegr",
+    "classif", "mlr3", "TaskClassif", "LearnerClassif", "PredictionClassif", "MeasureClassif"


Because left alignment only works with =, not with ~. You must right align here. See lorenzwalthert#2 (comment).

Edit: this should be resolved with latest styler.

pat-s · 2021-05-16T06:55:47Z

@lorenzwalthert The latest diff looks good to me! Thanks a lot for your help and persistence here 🙏

@mllg Are you fine with the diff and hence the style guide? If so, we can merge and I would update the documentation and inform all devs.

lorenzwalthert · 2021-05-16T09:49:10Z

What’s failing here @pat-s ? From a styling point of view, it seems fine now, agree?

pat-s · 2021-05-16T10:06:52Z

Some git checkout issues, seems random. I think we can ignore it.

Styling is fine 👍

lorenzwalthert · 2021-05-20T07:52:10Z

I think it was a GitHub outage. And now we have a merge conflict. I suggest to resolve it, retrigger styling and then get this merged before more merge conflicts accumulate.

pat-s · 2021-05-20T07:59:07Z

I'd like to have (at least) an approval by @mllg

mllg · 2021-05-20T08:07:57Z

Looks good to me. Thanks @pat-s and @lorenzwalthert for all the work you've put into this.

pat-s · 2021-05-20T08:22:31Z

Thanks. Given that the errors in CI are also appearing in the main branch and hence are unrelated to this PR, I'll merge this.

pat-s commented Apr 12, 2021

View reviewed changes

R/DataBackend.R Outdated Show resolved Hide resolved

pat-s commented Apr 12, 2021

View reviewed changes

lorenzwalthert mentioned this pull request Apr 13, 2021

Don't remove line break between } and ) mlr-org/styler.mlr#6

Closed

lorenzwalthert reviewed Apr 13, 2021

View reviewed changes

R/ResamplingBootstrap.R Outdated Show resolved Hide resolved

mllg reviewed Apr 13, 2021

View reviewed changes

lorenzwalthert mentioned this pull request Apr 14, 2021

Don't remove blank lines in function calls mlr-org/styler.mlr#10

Closed

lorenzwalthert mentioned this pull request Apr 14, 2021

Don't force blank line at top of function declaration mlr-org/styler.mlr#12

Closed

pat-s force-pushed the new-styler branch 2 times, most recently from 3451a84 to d63c553 Compare April 26, 2021 12:43

pat-s commented Apr 26, 2021

View reviewed changes

R/bibentries.R Show resolved Hide resolved

pat-s commented Apr 26, 2021

View reviewed changes

lorenzwalthert suggested changes Apr 26, 2021

View reviewed changes

pat-s force-pushed the new-styler branch from d63c553 to 4ab95db Compare April 30, 2021 11:08

restyle

983f42b

pat-s force-pushed the new-styler branch from 4ab95db to 983f42b Compare May 16, 2021 06:53

pat-s marked this pull request as ready for review May 16, 2021 06:55

pat-s added 2 commits May 20, 2021 09:57

Merge branch 'main' into new-styler

e819e2b

restyle

7ebb651

pat-s changed the title ~~New/updated mlr.styler package~~ Style package with new {mlr.styler} package May 20, 2021

pat-s merged commit 1c4c642 into main May 20, 2021

pat-s deleted the new-styler branch May 20, 2021 08:22

-                  }
-                ),
+                  }),

		new = c("bill_length", "bill_depth", "flipper_length", "body_mass")
		new = c("bill_length", "bill_depth", "flipper_length", "body_mass")

Style package with new {mlr.styler} package #626

Style package with new {mlr.styler} package #626

Conversation

pat-s commented Apr 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzwalthert Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzwalthert Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pat-s Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzwalthert Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzwalthert commented Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaphaelS1 commented Apr 13, 2021

pat-s commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzwalthert Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzwalthert Apr 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzwalthert May 16, 2021 • edited Loading

Choose a reason for hiding this comment

lorenzwalthert Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

pat-s commented May 16, 2021

lorenzwalthert commented May 16, 2021 • edited Loading

pat-s commented May 16, 2021

lorenzwalthert commented May 20, 2021

pat-s commented May 20, 2021

mllg commented May 20, 2021

pat-s commented May 20, 2021

lorenzwalthert Apr 14, 2021 •

edited

Loading

lorenzwalthert Apr 13, 2021 •

edited

Loading

pat-s Apr 26, 2021 •

edited

Loading

lorenzwalthert Apr 26, 2021 •

edited

Loading

lorenzwalthert commented Apr 12, 2021 •

edited

Loading

lorenzwalthert Apr 26, 2021 •

edited

Loading

lorenzwalthert Apr 30, 2021 •

edited

Loading

lorenzwalthert May 16, 2021 •

edited

Loading

lorenzwalthert Apr 26, 2021 •

edited

Loading

lorenzwalthert commented May 16, 2021 •

edited

Loading